Skip to content

combine foldConstants with standardizeTree#19

Open
mr-martian wants to merge 4 commits intoKhan:masterfrom
mr-martian:master
Open

combine foldConstants with standardizeTree#19
mr-martian wants to merge 4 commits intoKhan:masterfrom
mr-martian:master

Conversation

@mr-martian
Copy link
Contributor

I made stadardizeTree() do the job of foldConstants(), since there's really no need to recurse through the whole tree twice. I also made it fold binary nodes (e.g. 5*5 -> 25, "hi" === 3 -> false). If you don't want the added functionality, just comment out the if statement in lines 91-114.

As far as I can tell, this has resulted in a significant performance increase, from around 480 ms to run all the tests to approximately 390 ms. (alternatively, I could state this as: it's roughly back to where it was before I started messing with it [380 ms], though this does not take into account the impact of adding 22 more tests)

@mr-martian
Copy link
Contributor Author

Leaving it uncommented will result in the failure of test 4 of section 12 (Multiple multiple-var callbacks work.).
This is due to trying to match _ += $a + $b; against tree += 30 + 50 + 10;, after constant folding, this results in tree += 90, which of course, does not match.

@k4b7
Copy link
Contributor

k4b7 commented Jul 21, 2015

@mr-martian cool stuff. I will try to review this this weekend.

@k4b7
Copy link
Contributor

k4b7 commented Jul 27, 2015

@mr-martian I've only been able to have cursor look at your changes. With regards to the constant folding, could you pull those changes out for now. If we do decide to add constant folding I'd prefer to have it in a separate commit behind an option with tests.

I'm trying to understand in when constant folding would be helpful. Do you have some examples?

@mr-martian
Copy link
Contributor Author

I've removed the folding entirely.
I imagine it would be helpful in a situation where you were looking for x + 17 and the user for whatever reason entered x + 10 + 7

@k4b7
Copy link
Contributor

k4b7 commented Jul 27, 2015

I thought that lines 91-114 included new functionality that you were experimenting with. If I understand it correctly, those lines implement the same functionality that was in foldConstants plus you've added support for additional binary operators beyond just + and -.

> This is due to trying to match _ += $a + $b; against tree += 30 + 50 + 10;, after constant folding, this results in tree += 90, which of course, does not match.

This gave me the impress that constant folding itself was causing test failures, but since foldConstants was in the code before these changes and those tests were passing, it isn't constant folding, but rather the way it's implemented now.

If that's a fair assessment of the situation can you add the code back in and uncomment those lines and let's try to figure out what's causing those failures.

@k4b7
Copy link
Contributor

k4b7 commented Jul 27, 2015

Looking at the code for foldConstants I see that it was only folding unary expressions. Yeah, let's keep the binary expressions out. I'd like to be able to run those changes against the regression tests that we have for the challenges.

@mr-martian
Copy link
Contributor Author

Sorry if I was unclear, the functionality that was foldConstants is now the case "UnaryExpression": just below the commented lines. The commented (now removed) lines were only folding constants in binary and logical expressions.

@k4b7
Copy link
Contributor

k4b7 commented Aug 18, 2015

@mr-martian sorry for letting this stall. I want to run this against the integration tests for all of our challenges. Unfortunately, these tests aren't automated so it'll take some time. I'll have some time in a couple of weeks to do that.

made standardizeTree() directly modify tree, rather than deepClone() it at every level. Based on running the tests, this appears to now be as fast as it was before.

The other changes are due to accidentally working from a local copy of the main repo, rather than my fork.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants